-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Header for Overlay and Dialog #4138
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
d2d3fd4
to
ba95ab9
Compare
/** | ||
* Description text is displayed just below the header | ||
**/ | ||
description?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a question on the previous PR about why description|header|preheader
were limited to strings. (same applies to DialogHeader)
IMO string is the right choice here. The design is very deliberate about only allowing text content in these sections, and I don't think there's a scenario where we would want users to be able to inject any component they like here.
@joshwooding @origami-z @navkaur76 thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most cases, pure string would be provided anyway. However, in case teams want to put additional layout, icon or other relevant stuff, we could try to accommodate? A few other components does this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather use ReactNode or something similar. It's not worth being restrictive
box-sizing: border-box; | ||
} | ||
|
||
.saltOverlayHeader-body { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshwooding following up on question from previous PR: "Is "body" the correct way to refer to this?"
I think it is, as it's the body of the header (multiple text elements that are wrapped together, discrete from the end adornment). Not a strongly-help opinion, though, so happy to hear counter arguments 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be aligned with design @navkaur76
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In figma, the frame containing the main heading, pre-header and description is called "Block", if that helps? The accent bar sits outside of this frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closest block in the system is in stepper input?
/** | ||
* End adornment component | ||
*/ | ||
endAdornment?: ReactNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using endAdornment
to be consistent with other components. Are there any strong opinions about the name? @joshwooding @origami-z
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adornment is really only form controls previously... how does design refer to this? @navkaur76
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's "Close button" in figma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adornment is also used in grid corner triangle? @bhoppers2008
ba95ab9
to
744b89a
Compare
Replaced by other PRs. |
OverlayHeader
component to replace deprecatedOverlayPanelCloseButton
DialogHeader
to match updated Figma spec and deprecateDialogCloseButton